Derive shutdown scripts without blocking on wallet persistence by jkczyz · Pull Request #1011 · lightningdevkit/ldk-node

jkczyz · 2026-07-30T23:09:52Z

LDK invokes the sync SignerProvider::get_shutdown_scriptpubkey and get_destination_script callbacks on runtime worker threads while holding channel locks, e.g. from the event handler when accepting an inbound channel. These callbacks called Runtime::block_on to await wallet persistence, which can deadlock the runtime:

The event-handler task parks in block_on while holding the per-peer channel Mutex (its worker core is handed off via block_in_place).
The replacement worker picks up a lightning-net-tokio task whose PeerManager::process_events blocks synchronously on that same Mutex, capturing the core.
The task holding the wallet persister lock can then never be polled, so the awaited future never completes and no party can make progress.

Observed as permanent hangs of integration_tests_rust runs under load (the tests run the node on a single-worker runtime, where one captured core is fatal). See #1010 for full thread samples and analysis.

Instead of waiting on persistence, the wallet now keeps a small pool of pre-revealed addresses whose reveal is already persisted. The sync callbacks pop from the pool — no persistence work at all — and schedule a background refill. The refill reveals replacement addresses, persists the wallet change set, and only then publishes them for handout, so a handed-out script is always covered by persisted wallet state and a crash can never leave it unwatched by incremental chain syncs. The pool's derivation indices are persisted alongside the wallet, so a restart reloads the pooled addresses instead of revealing (and burning) fresh indices on every run.

Semantics worth calling out:

If wallet persistence fails for long enough, the pool drains and the callbacks fail closed: the channel open is rejected rather than handing out an address whose reveal isn't durable. The pool holds 16 addresses and is refilled after every handout; each channel open consumes two (destination + shutdown script), so persistence must stay unavailable across 8 consecutive opens before that happens.
The pooled addresses are revealed-but-unused, which widens what incremental syncs must watch by the pool size. Keeping the pool small and reloading it across restarts bounds that cost.

The gated-store test reproduces the deadlock: without the fix the acceptor wedges in get_shutdown_scriptpubkey and the open times out; with it the open completes from the pool and the refill's persistence lands once the store recovers. A restart test asserts that rebuilding a node from the same store performs no wallet writes (the pool is reloaded, not re-revealed) and that a channel open is served from the reloaded pool. Unit tests cover the publish-only-after-persist ordering, refill retry after a persistence failure without burning an extra index, validation of the persisted record on reload, crash replay from every store-write boundary, degradation to an empty pool on an undecodable record, and the fail-closed callbacks.

ldk-reviews-bot · 2026-07-30T23:09:55Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

tnull

Looks good I think, feel free to undraft.

tnull · 2026-08-06T08:36:47Z

+	/// the remaining workers, leaving none to drive the persistence future the callback waits on.
+	///
+	/// If the node crashes before the background flush lands, the revealed index is lost and the
+	/// address may be handed out again after restart. BDK's keychain lookahead still detects any


Hmm, so looking at this again one issue is that BDK's lookahead is not used for incremental syncs - there they really just check (all) previously revealed spks. So, if we fail persistence in the background task, we will miss a derived SPK, leading to address reuse (maybe acceptable in this narrow case) and only discovering transactions once it has been reused (maybe not acceptable as it could be considered funds loss, even if temporarily/recoverable - the user might not realize it's recoverable).

An alternative approach would be to create an AddressCache that is refreshed and persisted in the background, and returns Err(()) if no addresses are available. Something like:

At startup, derive perhaps 64 external addresses and persist the entire derivation range.

Only after persistence succeeds, place those addresses in an in-memory queue.

The synchronous signer callbacks pop an address without blocking.

Refill in the background below a low-water mark, publishing new addresses only after persistence succeeds.

If storage remains unavailable and the pool empties, return Err(()) and fail closed.

On top, as an additional robustness measure we might want to consider adding a wallet_persistence_pending dirty marker than would trigger a full scan on next restart whenever we're not certain all persistence operations have cleanly succeeded before stopping?

Thoughs?

Yeah, the cache is a better approach. Claude implemented it with some deviations:

🤖 Good catch — you're right, incremental syncs only query the revealed SPKs of the persisted wallet, so a crash before the deferred flush lands would leave the handed-out script unwatched. That's not acceptable, even if recoverable. I reworked the PR along the lines you sketched.

The callbacks now pop from a pool of addresses whose reveal is already persisted, and fail closed (Err(())) if the pool is empty. Newly revealed addresses are only published for handout after their change set persists; on failure they're retained and retried by the next refill, so no index is burned.

Two deviations from your sketch, both aimed at keeping the revealed-but-unused window small, since every pooled address widens what incremental syncs must watch:

Pool size 16 rather than 64, refilled after every handout rather than at a low-water mark. Since the refill runs after each pop, the pool size only bounds how many channel opens persistence can miss in a row before opens fail closed — 16 covers 8 consecutive opens (destination + shutdown script each), which seems plenty for an outage budget.

The pool's derivation indices are persisted (under bdk_wallet/address_pool) and reloaded on startup, after validating them against the wallet's last revealed index. Without that, every restart would burn a pool's worth of fresh indices, permanently growing the watched set. The record is written before the reveals' change set, so a crash between the two writes just re-derives the same indices on restart instead of stranding them (a unit test replays a reload from every store-write boundary to check this).

On the wallet_persistence_pending dirty marker: with the pool, unclean shutdown no longer risks an unwatched handed-out script, so I left it out of this PR — but it could still make sense as a general robustness measure for other in-flight wallet writes. Happy to explore it as a follow-up if you think it's worth it.

Codex:

The safer design is one FIFO external-address allocator with two consumption modes:

Synchronous signer callbacks pop immediately and accept the documented crash-reuse window.

Async public address requests pop from the same pool but persist the dequeue/refill before returning, preserving their current no-reuse guarantee.

🤖 Implemented exactly this split: the signer callbacks only pop and accept the documented bounded reuse across a crash, while get_new_address pops from the same pool but persists the dequeue before returning. Both consume from a single FIFO pool in reveal order.

tnull

I think if we going to do the address pool, we should redirect all calls to get_new_address through it, to avoid unnecessarily advancing the descriptor while still having unused ones lying around.

tnull · 2026-08-07T08:55:56Z

+/// unused tail.
+///
+/// [`DEFAULT_FULL_SCAN_STOP_GAP`]: crate::config::DEFAULT_FULL_SCAN_STOP_GAP
+pub(crate) const ADDRESS_POOL_TARGET_SIZE: usize = 16;


Codex:

[P1] Address pool can hide funds during seed recovery — /home/tnull/worktrees/ldk-node/pr-1011-upstream-20260807/src/wallet/mod.rs:96

The pool reveals 16 unused external addresses. A configured stop gap of 16 or less will therefore miss the first ordinary receive address during seed-only recovery. Even with the default gap of 20, two channel opens consume four still-unused scripts and refill through index 19; the next
receive address is index 20, beyond the full scan. The recovery strategy needs to account for pooled and handed-out-but-unused scripts, with a seed-restoration regression test.

🤖 Good catch. Full scans now extend the configured stop gap by the pool size (ADDRESS_POOL_SIZE), and get_new_address serves the oldest revealed address first. The seed-restore test isn't new — it failed against the unfixed pool and now pins the extended gap on both the Esplora and Electrum paths.

There's still a residual on the restore path: a wallet restored from seed can hand out addresses a previous installation already served. That's bounded address reuse, not a loss-of-funds issue, and it's documented. Happy to make the pool skip used indices after the first scan as a follow-up if you'd prefer.

tnull · 2026-08-07T08:59:23Z

+		let popped = self.address_pool.lock().expect("lock").available.pop_front();
+
+		let wallet = Arc::clone(self);
+		self.runtime.spawn_background_task(async move {


Codex:

[P2] Refill tasks accumulate for the node’s lifetime — /home/tnull/worktrees/ldk-node/pr-1011-upstream-20260807/src/wallet/mod.rs:612

Every callback adds a task to the runtime’s JoinSet, but that set is only drained during shutdown at /home/tnull/worktrees/ldk-node/pr-1011-upstream-20260807/src/runtime.rs:178. Completed refills therefore remain tracked—two per channel—and empty-pool failures still schedule more tasks.
Refill work should be coalesced into one in-flight task or completed tasks should be reaped.

This could probably be a spawn_cancellable_background_task? (for which we'll land a fix for the accumulation in #997)

🤖 Done — refills now spawn on the cancellable set. This needed one accompanying fix: an abort could drop a refill's taken reveals, so the refill now stages them with the persister in the same synchronous critical section that takes them from the wallet, with a regression test covering it. Note that completed tasks still sit in the cancellable set until shutdown (#997 will reap them continuously) — so the practical gain today is that shutdown aborts an in-flight refill immediately instead of waiting on it.

jkczyz

I think if we going to do the address pool, we should redirect all calls to get_new_address through it, to avoid unnecessarily advancing the descriptor while still having unused ones lying around.

🤖 Done — get_new_address now serves from the front of the pool and persists the dequeue before returning. One caveat: every handout still nets one fresh reveal; what this buys is allocation in reveal order, which keeps the unused window roughly pool-sized.

jkczyz · 2026-08-07T22:24:03Z

+/// unused tail.
+///
+/// [`DEFAULT_FULL_SCAN_STOP_GAP`]: crate::config::DEFAULT_FULL_SCAN_STOP_GAP
+pub(crate) const ADDRESS_POOL_TARGET_SIZE: usize = 16;


🤖 Good catch. Full scans now extend the configured stop gap by the pool size (ADDRESS_POOL_SIZE), and get_new_address serves the oldest revealed address first. The seed-restore test isn't new — it failed against the unfixed pool and now pins the extended gap on both the Esplora and Electrum paths.

There's still a residual on the restore path: a wallet restored from seed can hand out addresses a previous installation already served. That's bounded address reuse, not a loss-of-funds issue, and it's documented. Happy to make the pool skip used indices after the first scan as a follow-up if you'd prefer.

jkczyz · 2026-08-07T22:26:05Z

+		let popped = self.address_pool.lock().expect("lock").available.pop_front();
+
+		let wallet = Arc::clone(self);
+		self.runtime.spawn_background_task(async move {


🤖 Done — refills now spawn on the cancellable set. This needed one accompanying fix: an abort could drop a refill's taken reveals, so the refill now stages them with the persister in the same synchronous critical section that takes them from the wallet, with a regression test covering it. Note that completed tasks still sit in the cancellable set until shutdown (#997 will reap them continuously) — so the practical gain today is that shutdown aborts an in-flight refill immediately instead of waiting on it.

jkczyz · 2026-08-07T22:31:29Z

+	/// the remaining workers, leaving none to drive the persistence future the callback waits on.
+	///
+	/// If the node crashes before the background flush lands, the revealed index is lost and the
+	/// address may be handed out again after restart. BDK's keychain lookahead still detects any


🤖 Implemented exactly this split: the signer callbacks only pop and accept the documented bounded reuse across a crash, while get_new_address pops from the same pool but persists the dequeue before returning. Both consume from a single FIFO pool in reveal order.

tnull

Generally looks good, some comments.

tnull · 2026-08-10T08:49:31Z

+		// Extend the gap by the address pool size: the pool keeps that many addresses standing
+		// revealed-but-unused, which a scan restoring the wallet from seed alone would otherwise
+		// count against the configured gap.
+		(bounded as usize).saturating_add(ADDRESS_POOL_SIZE as usize)


I think with the FIFO design this (and below) is no longer needed.

If we'd want to account for the address pool size it should just be considered the minimum in clamp_full_scan_stop_gap rather than saturating_add to not unnecessarily slow down syncing in unexpected ways. But don't think we need this at all now.

🤖 Agreed, removed. Handouts consume the pool oldest-first, so pooled addresses sit past the handed-out ones, where they can't make a full scan stop short of funds.

One qualifier: a handout that fails while a concurrent one proceeds can briefly leave a pooled address below a handed-out one. The inversion is bounded by the pool size and consumed by the next handouts, so the default gap of 20 covers it — but a configured gap below the pool size could stop short during that window. Happy to make the pool size the minimum in clamp_full_scan_stop_gap, as you suggested, if you'd like that covered too.

Chasing this down also turned up a bug, fixed in a separate fixup: the refill flushed its reveals even when the pool-record write failed, permanently skipping those indices and widening what a restore from seed must scan across.

One narrow residual remains: reveals retained after a failed record write can still be flushed by a later unrelated persist, and after a restart they stay revealed-but-unused. Re-pooling them at load turns out unsound — a full scan (or an older version of the software) can also reveal indices past the record, and re-pooling those would reuse handed-out or even used addresses — so I left the residual in place: it's bounded by the pool size and needs a compound failure. Could revisit with a tx-graph check if you think it's worth closing.

tnull · 2026-08-10T09:16:32Z

+				} else {
+					log_error!(
+						self.logger,
+						"Dropping persisted address pool index {} beyond the wallet's last revealed index",


AFAIU we should never hit this under normal operation? Should we add a debug_assert here?

🤖 We do hit it: the record is deliberately written before the wallet's change set, so a crash between the two writes — or a failed wallet write followed by a restart — leaves the record listing indices the wallet never revealed. This branch is the recovery for that, and two of the unit tests exercise it, so a debug_assert would fail them.

What does hold is that a valid record never points more than the pool size past the wallet's last revealed index. Happy to add that as a debug_assert if you'd like.

tnull · 2026-08-10T09:17:56Z

+			for index in persisted_indices {
+				// Only trust indices the persisted wallet actually revealed: anything beyond
+				// `last_revealed` would hand out a script no chain sync path watches.
+				if last_revealed.map_or(false, |last| index <= last) {


Why is this extra check needed? Couldn't we just persist the whole AddressPool, persisting only the available field? I guess that's more data, but would allow us to avoid the extra AddressPoolRecord?

🤖 The check is needed because the record and the wallet's change set are two separate keys in the KV store, and KVStore can't write two keys atomically. We write the record first, so a crash between the two writes leaves the record listing indices the wallet's persisted state never revealed — handing those out would give out scripts no sync path watches.

Persisting the full addresses instead of the indices wouldn't change that: we'd still have to check each entry against the wallet, it would only save the (cheap) re-derivation. The one way to remove the check entirely is to store the pool in the same key as the wallet's own indexer state, so both land in one write — but that changes the store format. Could do that as a follow-up if you prefer.

tnull · 2026-08-10T09:19:57Z


+	// Fill the address pool up front so LDK's sync `SignerProvider` callbacks can hand out
+	// pre-persisted addresses without waiting on wallet persistence.
+	runtime.block_on(wallet.initialize_address_pool()).map_err(|e| {


We need to parallelize the reads on startup.

IMO it would also preferable to keep up the pre-existing patterns of doing the read_ methods here in parallel, and then hand that in to initialize via an AddressPool::new or so rather than doing all/obfuscating in an initialize_ method.

🤖 Done. The record read now runs in the tokio::join! with the other startup reads, its result is handed to Wallet::new, which validates it in AddressPool::new, and the builder only blocks on the initial pool top-up. One side effect: a failed read now fails the build with ReadFailed rather than WalletSetupFailed.

tnull

LGTM, please squash

LDK invokes the sync `SignerProvider::get_shutdown_scriptpubkey` and `get_destination_script` callbacks on runtime worker threads while holding channel locks, e.g. when accepting an inbound channel. Blocking there on wallet persistence could deadlock the runtime: the parked callback still held the channel locks, other tasks blocking synchronously on those locks captured the remaining worker cores, and the persistence future the callback waited on could then never be polled. Observed as a permanent hang of integration test runs. Instead, the wallet now keeps a small pool of addresses whose reveals are already durably persisted. The callbacks pop from the pool and schedule a background refill, failing closed (rejecting the channel) if the pool is exhausted. Refills publish newly revealed addresses for handout only after their reveal has persisted, so a handed-out script is always covered by persisted wallet state and stays watched by incremental chain syncs even across a crash. `get_new_address` serves from the same pool, making the handout durable before returning to preserve its no-reuse guarantee. The pool's derivation indices are persisted and reloaded on startup, so restarts don't burn fresh indices on every run. The record is written before the reveals it references: a crash between the writes then re-derives the same indices rather than stranding revealed indices no record covers. Addresses are handed out oldest first, keeping the pooled addresses beyond the handed-out ones, where they cannot hide funds from a from-seed restore's full-scan stop gap. Such a restore refills the pool before any scan runs, so it may re-serve addresses a previous installation handed out — bounded address reuse, not fund loss. Shutdown aborts an in-flight refill rather than waiting on it. Fixes lightningdevkit#1010. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

jkczyz mentioned this pull request Jul 30, 2026

Runtime deadlock in sync SignerProvider callbacks #1010

Closed

tnull added this to the 0.8 milestone Aug 5, 2026

tnull reviewed Aug 5, 2026

View reviewed changes

jkczyz marked this pull request as ready for review August 5, 2026 15:54

jkczyz requested a review from tnull August 5, 2026 15:54

tnull reviewed Aug 6, 2026

View reviewed changes

jkczyz requested a review from tnull August 6, 2026 22:03

tnull reviewed Aug 7, 2026

View reviewed changes

jkczyz commented Aug 7, 2026

View reviewed changes

jkczyz requested a review from tnull August 7, 2026 22:47

tnull reviewed Aug 10, 2026

View reviewed changes

jkczyz requested a review from tnull August 10, 2026 23:26

tnull reviewed Aug 11, 2026

View reviewed changes

jkczyz force-pushed the 2026-07-signer-blockon-deadlock branch from a7c52af to d8f1b9d Compare August 11, 2026 16:02

jkczyz requested a review from tnull August 11, 2026 16:21

tnull approved these changes Aug 11, 2026

View reviewed changes

tnull merged commit d7ec0cf into lightningdevkit:main Aug 11, 2026
17 of 25 checks passed

Uh oh!

Conversation

jkczyz commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnull Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jkczyz commented Jul 30, 2026 •

edited

Loading

ldk-reviews-bot commented Jul 30, 2026 •

edited

Loading

tnull Aug 6, 2026 •

edited

Loading